Add client completion notification and details#1368
Add client completion notification and details#1368stephentoub wants to merge 5 commits intomodelcontextprotocol:mainfrom
Conversation
src/ModelContextProtocol.Core/Client/StdioClientCompletionDetails.cs
Outdated
Show resolved
Hide resolved
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
I would like to see this implemented for stateful Streamable HTTP when there are 404s as well, but I think this is already a nice improvement even without that part. I have a small preference for the abstract Completion property over the method, but I don't care too much either way on that.
src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs
Outdated
Show resolved
Hide resolved
Introduce a new mechanism for reporting MCP client session completion details, including both graceful and error terminations. Add a Completion property to McpClient, exposing a `Task<ClientCompletionDetails>` that completes with details about session closure. Implement extensible completion details (including StdioClientCompletionDetails for stdio transports) and propagate them via a new internal TransportClosedException.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
stephentoub
dismissed stale reviews from halter73 and eiriktsarpalis
via
860e132
stephentoub
force-pushed
the
completiondetails
branch
from
cedcddc to
860e132
Compare
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
It turns out 10x wasn't enough 😭
Failed Checks: - ClientRespectsRetryField: Client MUST respect the retry field, waiting the given number of milliseconds before attempting to reconnect Error: Client reconnected very late (5355ms instead of 500ms). Client appears to be ignoring the retry field and using its own backoff strategy.
https://github.com/modelcontextprotocol/csharp-sdk/actions/runs/22415043174/job/64898373590?pr=1368
| // after the call to Cancel below, to invoke SetDisconnected. | ||
| _disconnectError = new TransportClosedException(new HttpClientCompletionDetails | ||
| { | ||
| HttpStatusCode = statusCode, |
There was a problem hiding this comment.
@copilot Can you please remove the HttpStatusCode parameter from SetSessionExpired and replace this line with just HttpStatusCode = HttpStatusCode.NotFound? That's only assuming that this is only called given a 404 status code which appears to be that case.
| if (_options.TransportMode is not HttpTransportMode.AutoDetect || _getReceiveTask is not null) | ||
| { | ||
| SetDisconnected(); | ||
| SetDisconnected(_disconnectError ?? new TransportClosedException(new HttpClientCompletionDetails())); |
There was a problem hiding this comment.
@copilot Assuming that the ?? new TransportClosedException(new HttpClientCompletionDetails() logic should be unreachable, can use configure the HttpClientCompletionDetails with an exception stating as much? If not, can you configure the HttpClientCompletionDetails with an exception describing how the client might have disconnected?
I think there's a real pre-existing bug lurking. |
4 participants
Introduce a new mechanism for reporting MCP client session completion details, including both graceful and error terminations. Add a Completion property to McpClient, exposing a
Task<ClientCompletionDetails>that completes with details about session closure. Implement extensible completion details (including StdioClientCompletionDetails for stdio transports) and propagate them via a new internal TransportClosedException.Closes #1332
Closes #438